Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

docs(Tooltip): add a11y docs #2892

Merged
merged 7 commits into from
Jul 7, 2022

Conversation

thatblindgeye
Copy link
Contributor

@thatblindgeye thatblindgeye commented Mar 31, 2022

Closes #2596

Tooltip a11y tab

Current implementation for a11y docs on the tooltip. This is based off of the current changes made in patternfly/patternfly-react#7332 and patternfly/patternfly-react#7335.

Additionally, I added in some verbiage in regards to tooltips on static elements. In #1108 (comment) it was mentioned that tooltips on static elements shouldn't be recommended. However, utilizing the pattern of aria-live instead of aria-describedby does seem to resolve the gaps mentioned.

Original comment:

One proposal I want to put out there is whether we should include verbiage for when a tooltip shouldn't be announced to screen reader users. Specifically, if an element already has a label (such as via aria-label), and the tooltip contents is essentially the same thing, should the tooltip then be prevented from being announced to screen reader users to avoid redundant/duplicate announcements? Or would the better approach be to avoid adding an aria-label if a tooltip is to be attached (as we've seen with using React refs, tooltips may not always be announced, though I've noticed a possible fix could be using aria-live on tooltips attached via ref)?

Using the Tooltip on icon example, if the example code was changed so that the button had aria-label="Copy contents to clipboard", a screen reader could announce both that and the tooltip contents, which would be redundant information. This could also be trying to solve a problem that may not really exist or not be a significant issue, though.

@patternfly-build
Copy link
Contributor

patternfly-build commented Mar 31, 2022

@thatblindgeye thatblindgeye marked this pull request as ready for review May 4, 2022 19:43
nicolethoen
nicolethoen previously approved these changes May 9, 2022
jessiehuff
jessiehuff previously approved these changes May 9, 2022
Copy link
Contributor

@jessiehuff jessiehuff left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! Thanks for working on this! 🙂

@thatblindgeye
Copy link
Contributor Author

@nicolethoen from the backlog meeting earlier, I noticed one issue regarding Popover and adding hover functionality. After digging through that, based on issue #2871 + conversation in the issue linked in it, do you think it would be better to update the a11y docs for Tooltips taking into account these intended changes to Tooltip + Popover? For the most part may just be a case of re-adding the recommendation to not use tooltips on static elements, and then later adding in any other verbiage the design guidelines end up including.

@nicolethoen
Copy link
Collaborator

For the most part may just be a case of re-adding the recommendation to not use tooltips on static elements, and then later adding in any other verbiage the design guidelines end up including.

I think I agree - we should include the recommendation of not using tooltips on static elements and then we can follow up if recommendations evolve.
The only exception being if we already are following the 'yet to be written' recommendations in the examples/demos and then it would just mean that the design guidelines would be one step behind the a11y docs which is not the end of the world.

@thatblindgeye thatblindgeye dismissed stale reviews from jessiehuff and nicolethoen via 56ce05e May 11, 2022 13:34
@thatblindgeye
Copy link
Contributor Author

Updated the docs a bit based on convo above. I also included some verbiage for when one might use labelledby or describedby.

@nicolethoen
Copy link
Collaborator

@thatblindgeye I think the information here is very helpful - but it is a lot of information. I wonder if each or most of your bullet points could benefit from a snippet of html or react to demonstrate what you are describing. Sort of link how we do on the website when talking about icons accessibility

Copy link
Contributor

@jessiehuff jessiehuff left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I really love these updates! I think it's a good call out that you're differentiating labeling from supplementary information.

I think Nicole makes a good suggestion as well. Adding small snippets/examples helps make the points clearer and more consumable.

@thatblindgeye
Copy link
Contributor Author

@nicolethoen @jessiehuff I updated the tooltip doc with the new template. I also added in some code blocks/examples for certain things throughout. For examples with specific props, they're not exactly inline in the table and instead placed in sub-sections, so let me know what you think of that sort of layout or if you have other ideas for how to go about them

Copy link
Contributor

@jessiehuff jessiehuff left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the new template for this, but I find the top section a little confusing between the React and HTML/CSS bullet points. At first I thought that you meant that everyone needs to add all of those things. For example, I saw the point about adding the role and thought, "isn't that already added for you?" Then I realized that you meant if you're just using the HTML/CSS, not that you need to add HTML/CSS. Admittedly I think previously I just included the React suggestions before and kept the HTML/CSS in the customization section. However, I can understand why it might make sense to keep at the top, but I'd be curious what others think. Do you think it would be helpful for us to separate that into two different categories at the top to keep it clear? Or maybe it was just me that got confused haha.

@thatblindgeye
Copy link
Contributor Author

thatblindgeye commented Jul 5, 2022

Or maybe it was just me that got confused haha.

I definitely think you raise a valid point. What do you think of something along the lines of this instead:

- Pass in `role="tooltip"` to the element acting as the tooltip component if using the HTML/CSS library
- Pass in `aria="labelledby"` to the tooltip component if using the PatternFly React library, or the `aria-labelledby` attribute to the trigger if using the HTML/CSS library, when the tooltip should act as the primary label for its trigger

or use nested bullets to separate the two libraries:

- Pass in `aria="labelledby"` to the tooltip component if using the PatternFly React library when the tooltip should act as the primary label for its trigger.
    - Pass in the `aria-labelledby` attribute to the trigger if using the HTML/CSS library

The "if using X library..." could also be placed first, e.g. "If using the HTML/CSS library, pass in role="tooltip"...".

If we did want separate bulleted lists, we could do something along the lines of:

To implement an accessible PatternFly tooltip:

- ...

For the PatternFly React library:

- ...

For the HTML/CSS library:

- ...

@nicolethoen
Copy link
Collaborator

I think I like your second suggestion more. maybe something like:

To implement an accessible PatternFly tooltip:

  • Avoid using tooltips on static elements such as a div or span, except in cases of truncation.

Using react:

  • Pass in aria="labelledby" when the tooltip should act as the primary label for its trigger (with snippet)

Using HTML/CSS:

  • Set the aria-labelledby attribute on the trigger when the tooltip should act as the primary label for its trigger. (code snippet)
  • Pass in the aria-describedby attribute to the trigger (HTML/CSS) when the tooltip should act as supplementary information. (code snippet)

nicolethoen
nicolethoen previously approved these changes Jul 6, 2022
@nicolethoen nicolethoen merged commit 80f6717 into patternfly:main Jul 7, 2022
mcarrano added a commit to mcarrano/patternfly-org that referenced this pull request Jul 19, 2022
…nto text-input-group

* 'main' of https://github.com/patternfly/patternfly-org: (170 commits)
  chore(release): releasing packages [ci skip]
  Updates button info architecture to align with guidelines. (patternfly#3061)
  Card design guideline changes. (patternfly#3087)
  chore(release): releasing packages [ci skip]
  iss3078 (patternfly#3079)
  chore(release): releasing packages [ci skip]
  changing alert guidelines (patternfly#3064)
  chore(release): releasing packages [ci skip]
  mastshead (patternfly#3062)
  chore(release): releasing packages [ci skip]
  jumplink (patternfly#3051)
  chore(release): releasing packages [ci skip]
  calendar (patternfly#3058)
  chore(release): releasing packages [ci skip]
  feat(doc-framework): add dark theme toggle for full page examples (patternfly#3056)
  chore(release): releasing packages [ci skip]
  edit overflow guidelines (patternfly#3038)
  chore(release): releasing packages [ci skip]
  docs(a11y): create template for a11y docs (patternfly#3019)
  docs(Tooltip): add a11y docs (patternfly#2892)
  ...

# Conflicts:
#	packages/v4/patternfly-docs/content/design-guidelines/components/text-input-group/img/attribute-value-filter.png
#	packages/v4/patternfly-docs/content/design-guidelines/components/text-input-group/img/auto-complete-search.png
#	packages/v4/patternfly-docs/content/design-guidelines/components/text-input-group/img/text-input-elements.png
#	packages/v4/patternfly-docs/content/design-guidelines/components/text-input-group/text-input-group.md
jessiehuff pushed a commit to jessiehuff/patternfly-org that referenced this pull request Oct 24, 2022
* docs(Tooltip): add a11y docs

* Update documentation

* Align verbiage with intended design

* Convert doc to align with new guidelines

* Update docs to new template

* Separate instructions for libraries

* Update a11y section in design guidelines
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add a11y documentation for Tooltip
4 participants